Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

report a command system origin in case of panic #2222

Closed
wants to merge 5 commits into from

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented May 19, 2021

related to #1743 and #2219

Add a new feature bevy_command_panic_origin. If it is enabled, the command system origin will be logged in case of panic when applying it.

This is not meant to be a final feature with nice UX but more a stepping stone for adding proper result handling of commands. But it can be useful as is, and adding error to all commands is a much larger change #2004

@mockersf mockersf force-pushed the command-panic-origin branch from ca9467f to 4c87867 Compare May 19, 2021 23:12
@mockersf mockersf added the A-ECS Entities, components, systems, and events label May 19, 2021
crates/bevy_ecs/src/system/commands.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/into_system.rs Outdated Show resolved Hide resolved
@NathanSWard NathanSWard added the C-Usability A targeted quality-of-life change that makes Bevy easier to use label May 19, 2021
@NathanSWard NathanSWard self-requested a review May 20, 2021 00:18
commands: Vec<Box<dyn Command>>,
pub(crate) commands: Vec<Box<dyn Command>>,
#[cfg(feature = "command_panic_origin")]
pub(crate) system_name: Option<Cow<'static, str>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be Option<_>?
inside fn init system_name is always set as Some.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the struct CommandQueue is pub and can be built using default() (done in LightsNode and in RenderResourcesNode). In those case a None is better than an empty string I think.

Though with the field behind a feature it's less of an issue I guess

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just impl Default for CommandQueue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless it's actually possible for it to be None...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be None!

Co-authored-by: bjorn3 <[email protected]>
@MinerSebas
Copy link
Contributor

The new feature also needs to be mentioned in the ./docs/cargo_features.md file.

@NathanSWard
Copy link
Contributor

@mockersf
Curious on what your thoughts are with this and how #2241 interact?
Specifically because my PR changes the default behavior to not panic, so the panic handler would only work where someone specifies

.on_err(CommandErrorHandler::panic())

which, I have a feeling will be few and far between.

It's probably a better approach to have the Command trait be defined as something like:

struct CommandArgs<'a> {
    world: &'a mut World,
    system_name: Cow<'static, str>,
}

trait Command  {
    fn write(self: Box<Self>, args: CommandArgs);
}

So that way all commands (and error handlers) can have the system name correctly propagated to them?

@mockersf mockersf marked this pull request as draft May 28, 2021 21:54
@mockersf
Copy link
Member Author

Curious on what your thoughts are with this and how #2241 interact?

I think this'll definitely to be looked at after #2241 has been merged, moved it to draft

It's probably a better approach to have the Command trait be defined as something like:

struct CommandArgs<'a> {
    world: &'a mut World,
    system_name: Cow<'static, str>,
}

trait Command  {
    fn write(self: Box<Self>, args: CommandArgs);
}

So that way all commands (and error handlers) can have the system name correctly propagated to them?

Or having the system name in the CommandError struct?

@NathanSWard
Copy link
Contributor

Or having the system name in the CommandError struct?

Yep, it'll be there to.
However, to have it even be passed to the error handler, it needs to be passed though to Command::write as well.

@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@mockersf mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021
@mockersf mockersf closed this Oct 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants